Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

change all url strings to URI type #79

Merged
merged 6 commits into from
Sep 11, 2015
Merged

Conversation

DarthKipsu
Copy link
Contributor

Fixes #77

@DarthKipsu DarthKipsu changed the title change all url string to URI type change all url strings to URI type Sep 5, 2015
@@ -6,6 +6,7 @@

import com.google.gson.JsonArray;
import com.google.gson.JsonObject;
import java.net.URI;

import java.util.Map;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: The empty line should be between the java and com.google import groups, not inside the java group.

@ljleppan
Copy link
Member

ljleppan commented Sep 5, 2015

Added some (Mostly small and superficial) comments. Nice work!

For details on import ordering, see https://google.github.io/styleguide/javaguide.html#s3.3.3-import-ordering-and-spacing. Just mentally replace com.google with fi.helsinki :)

@@ -13,6 +13,7 @@

import com.github.tomakehurst.wiremock.client.WireMock;
import com.github.tomakehurst.wiremock.junit.WireMockRule;
import java.net.URI;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Import order

@ljleppan
Copy link
Member

ljleppan commented Sep 5, 2015

Two more import order notes, LGTM when those are fixed.

@@ -61,13 +61,13 @@ public Course call() throws TmcCoreException, URISyntaxException {

if (!course.isPresent()) {
throw new TmcCoreException(
"Attempted to fetch nonexistent course " + urlWithApiVersion);
"Attempted to fetch nonexistent course " + urlWithApiVersion);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indented with tabs?

}
return argument.toString().contains(search);
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indented with tabs, lets make it to use spaces (you can configure your IDE not to use tabs)

@DarthKipsu
Copy link
Contributor Author

Could you please merge this or comment if there's still something to review.

@ljleppan
Copy link
Member

Sorry for the delay, we only get a notice when there's a new comment, not when you push new commits to the branch.

LGTM.

We'll merge after @jamox has taken a look as well (likely during tomorrow).

@jamo
Copy link
Member

jamo commented Sep 11, 2015

LGTM, nice work.

jamo added a commit that referenced this pull request Sep 11, 2015
Change all uri like strings to URI type
@jamo jamo merged commit 097db2e into testmycode:master Sep 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants